Skip to content

Do not process forwarded commands until node is part of the network#7936

Merged
achamayou merged 12 commits into
mainfrom
copilot/forwarded-messages-logic
Jun 12, 2026
Merged

Do not process forwarded commands until node is part of the network#7936
achamayou merged 12 commits into
mainfrom
copilot/forwarded-messages-logic

Conversation

Copilot AI commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Forwarded commands received in recv_node_inbound (src/node/node_state.h) were dispatched to cmd_forwarder->recv_message(...) unconditionally, bypassing the "part of network" state guard applied to all other node-to-node messages. A node could therefore execute a forwarded command while still in an early startup state, with potentially undefined behaviour for some commands.

Changes

  • src/node/node_inbound_message.h: New header-only helper recv_node_inbound_message, which reads a serialised node_inbound message and gates dispatch behind the "part of network" state check, so forwarded commands are gated identically to channel and consensus messages — processed only in partOfNetwork, partOfPublicNetwork, or readingPrivateLedger. Messages arriving too early are dropped with the same LOG_DEBUG_FMT as other node messages. The guard uses the new StateMachine::check_one_of() helper.
  • src/node/node_state.h: recv_node_inbound now delegates to the new recv_node_inbound_message helper. The previously-unreachable LOG_FAIL_FMT in the forwarded_msg case is replaced by the real handler; dispatch logic is otherwise unchanged.
  • src/ds/state_machine.h: Added check_one_of(const std::set<T>&), which loads the atomic state once and checks membership against the provided set of states, avoiding repeated atomic reads when checking whether the node is in one of several states.
  • src/ds/test/state_machine.cpp / src/node/test/node_inbound_message.cpp / CMakeLists.txt: Added two C++ unit tests (state_machine_test and node_inbound_message_test).
  • CHANGELOG.md / python/pyproject.toml: Added a new ## [7.0.6] / Fixed entry (with its release-tag link) and bumped the Python package version to 7.0.6 to match, so the "Release notes" CI check (which requires the first CHANGELOG version to match pyproject.toml) passes. The entry references this PR, #7936.
// Only process messages once part of network. This includes forwarded
// commands, which must not be executed until the node is part of the
// network, as some commands may otherwise exhibit undefined behaviour.
if (!sm.check_one_of(
      {NodeStartupState::partOfNetwork,
       NodeStartupState::partOfPublicNetwork,
       NodeStartupState::readingPrivateLedger}))
{
  LOG_DEBUG_FMT("Ignoring node msg received too early - current state is {}", sm.value());
  return;
}

switch (msg_type)
{
  case forwarded_msg:
    cmd_forwarder.recv_message(from, payload_data, payload_size);
    return;
  // channel_msg, consensus_msg, default unchanged
}

Testing

  • state_machine_test (src/ds/test/state_machine.cpp): exercises the StateMachine API and the new check_one_of helper, including empty-set and state-transition edge cases.
  • node_inbound_message_test (src/node/test/node_inbound_message.cpp): drives recv_node_inbound_message with a serialised node_inbound message and stub forwarder/channel/consensus handlers. Asserts that in early states (uninitialized, initialized, pending, readingPublicLedger) a forwarded_msg is not dispatched, and that in partOfNetwork, partOfPublicNetwork, and readingPrivateLedger it is dispatched with the expected from/payload. Also covers channel_msg and consensus_msg being gated identically.
  • Both unit tests were built and run locally with the project's CMake/Ninja toolchain, with all assertions passing.
  • Python e2e: not included - harder and racy, since it would require forwarding a write to a node mid-startup, whereas nodes accepting n2n traffic are normally already part of the network. The unit-test approach covers the gating logic more reliably.
  • Release notes, C/C++ format, CMake format and prettier checks all pass locally.

Copilot AI changed the title [WIP] Fix forwarded messages processing until network is open Do not process forwarded commands until node is part of the network Jun 9, 2026
Copilot AI requested a review from achamayou June 9, 2026 20:05
@achamayou

Copy link
Copy Markdown
Member

@copilot add the C++ unit test, and add a check_one_of() to the StateMachine so we can pass a std::set and avoid calling the atomic multiple times when we check if we are in one of the set of states.

Copilot AI commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

@copilot add the C++ unit test, and add a check_one_of() to the StateMachine so we can pass a std::set and avoid calling the atomic multiple times when we check if we are in one of the set of states.

Done in the latest commit:

  • Added StateMachine::check_one_of(const std::set<T>&) which loads the atomic once and checks membership, and used it in recv_node_inbound.
  • Added a C++ unit test src/ds/test/state_machine.cpp (registered as state_machine_test) covering check, expect, advance, value and check_one_of (including empty-set and state-transition cases).

@achamayou

Copy link
Copy Markdown
Member

@copilot please add this unit test as well:

C++ unit test: drive recv_node_inbound directly with a serialised forwarded_msg. With the state machine in an early state (initialized/pending), assert the command is not dispatched (via a stub forwarder handler / absence of response); transition to each of partOfNetwork, partOfPublicNetwork, readingPrivateLedger and assert it is processed.

Copilot AI commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

@copilot please add this unit test as well:

C++ unit test: drive recv_node_inbound directly with a serialised forwarded_msg. With the state machine in an early state (initialized/pending), assert...

Added in src/node/test/node_inbound_message.cpp (node_inbound_message_test). To make recv_node_inbound drivable in a unit test without constructing a full NodeState, the gating + dispatch was extracted into a header-only helper recv_node_inbound_message (src/node/node_inbound_message.h), which recv_node_inbound now calls. The test serialises a node_inbound message and drives the helper with stub forwarder/channel/consensus handlers:

  • early states (uninitialized, initialized, pending, readingPublicLedger): forwarded command is not dispatched;
  • partOfNetwork, partOfPublicNetwork, readingPrivateLedger: it is dispatched with the expected from/payload.

It also covers channel_msg and consensus_msg being gated identically. I built and ran both new unit tests locally (all assertions pass), and fixed a doctest main linking error in the new single-file tests.

@achamayou

Copy link
Copy Markdown
Member

@copilot the formatting, please.

Copilot AI commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

@copilot the formatting, please.

The failing check was "Release notes": scripts/extract-release-notes.py requires the first version in CHANGELOG.md to match python/pyproject.toml, and ## [Unreleased] did not. Replaced it with a proper ## [7.0.6] section (with the release-tag link) and bumped pyproject.toml to 7.0.6 to match. Release notes, C/C++ format, CMake format and prettier checks all pass locally now (84f1c42).

@achamayou achamayou marked this pull request as ready for review June 10, 2026 13:40
@achamayou achamayou requested a review from a team as a code owner June 10, 2026 13:40
Copilot AI review requested due to automatic review settings June 10, 2026 13:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tightens node-to-node message handling by ensuring forwarded commands are only dispatched once the node is in a “part of network” startup state, aligning forwarded command gating with existing channel/consensus message gating.

Changes:

  • Introduces recv_node_inbound_message helper to parse and dispatch node_inbound messages behind the startup-state guard.
  • Adds StateMachine::check_one_of() plus new unit tests covering the new helper and state-machine API.
  • Updates release metadata (CHANGELOG + Python package version) for a 7.0.6 patch release.

Custom instructions used:

  • .github/copilot-instructions.md
  • .github/instructions/reviewing.instructions.md
  • .github/instructions/changelog.instructions.md

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/node/node_inbound_message.h Adds shared dispatch helper to gate forwarded/channel/consensus node messages behind startup-state checks.
src/node/node_state.h Routes recv_node_inbound through the new helper.
src/ds/state_machine.h Adds check_one_of() helper for checking membership in multiple allowed states.
src/ds/test/state_machine.cpp Adds unit coverage for StateMachine basics and check_one_of().
src/node/test/node_inbound_message.cpp Adds unit coverage ensuring inbound messages (including forwarded) are dropped/processed based on startup state.
CMakeLists.txt Registers the two new unit test executables.
CHANGELOG.md Adds a 7.0.6 “Fixed” entry referencing #7936.
python/pyproject.toml Bumps Python package version to 7.0.6 to match the CHANGELOG.

Comment thread src/node/node_state.h
Comment thread src/node/node_inbound_message.h Outdated
Comment thread src/ds/state_machine.h
achamayou and others added 2 commits June 10, 2026 15:04
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
Copilot AI requested a review from achamayou June 10, 2026 17:56
@achamayou achamayou enabled auto-merge (squash) June 12, 2026 17:05
@achamayou achamayou merged commit 0022530 into main Jun 12, 2026
19 checks passed
@achamayou achamayou deleted the copilot/forwarded-messages-logic branch June 12, 2026 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Forwarded messages maybe shouldn't be processed until the network is open

4 participants